added validation around code_challenge_method to reject non S256 values#1599
added validation around code_challenge_method to reject non S256 values#1599bwang-icf wants to merge 13 commits into
Conversation
jimmyfagan
left a comment
jimmyfagan
left a comment
There was a problem hiding this comment.
Looks good, just some comments to hopefully make this a bit cleaner hopefully.
| # Validating before medicare login step that code_challenge_method is correct as per BB2-4805 | ||
| # We could consider adding other checks at this step as well. Open to the team's suggestions |
There was a problem hiding this comment.
I'd prefer keeping these kinds of comments out of the code, feel free to start convos in the PR, but I don't think these comments are useful to potentially stay in the code for a while after merging.
There was a problem hiding this comment.
This can be removed for sure.
| validation_error = self._validate_code_challenge_method(code_challenge_method) | ||
| if validation_error: | ||
| return validation_error |
There was a problem hiding this comment.
Feels like this is probably better coming after the _check_for_required_parameters call. Check that the required params exist before checking for validity of values.
| def _validate_code_challenge_method(self, code_challenge_method): | ||
| """Validate code_challenge_method is S256 if provided. | ||
|
|
||
| Returns None if valid, JsonResponse error if invalid. | ||
| """ | ||
| if code_challenge_method and code_challenge_method != 'S256': | ||
| return JsonResponse( | ||
| { | ||
| 'status_code': HTTPStatus.BAD_REQUEST, | ||
| 'error': 'invalid_request', | ||
| 'error_description': f'code_challenge_method must be S256, got: {code_challenge_method}', | ||
| }, | ||
| status=HTTPStatus.BAD_REQUEST, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Wondering if we can move this to _check_for_required_params similar to what we do when we confirm the existence of state and then validate it in an elif. And then maybe we can add a call to _check_for_required_params to dispatch so we do these checks pre-login as you suggested. How does that sound? If we do that, it might invalidate one of my other comments.
There was a problem hiding this comment.
Yeah, I think it can be there. That answers my other comment from before about where else that check might live. I'll change it so it's part of the required_param check
| return validation_error | ||
| kwargs['code_challenge'] = request.POST.get('code_challenge') | ||
| kwargs['code_challenge_method'] = request.POST.get('code_challenge_method') | ||
| kwargs['code_challenge_method'] = code_challenge_method |
There was a problem hiding this comment.
Was this necessary? Kind of unclear to me why any of the kwargs need to be manually set like this to be honest.
There was a problem hiding this comment.
I think otherwise the PKCE params don't show up in the super().post. It needs to be set within kwargs.
There was a problem hiding this comment.
Why is it needed in super().post? Seems like it was working fine without this before
There was a problem hiding this comment.
This could be reverted back, but I was mostly just following what was currently within the code. I think currently the code in master does set the kwargs here, but I can definitely try to run this without setting kwargs and see what happens and remove these if they're really not needed.
There was a problem hiding this comment.
Looks like these indeed are not needed anymore, so we're good removing this. Some chance for related refactors in form_valid to uncomment the lines 398-399, and then removing the ifs for those values beneath that. Seems like these are all kind of remnants of when PKCE was first added, so some cleanup in this area would be nice while we're at it.
Agent-Logs-Url: https://github.com/CMSgov/bluebutton-web-server/sessions/ec2649c7-d427-4f59-ad18-098613b75e4e Co-authored-by: bwang-icf <178809349+bwang-icf@users.noreply.github.com>
Agent-Logs-Url: https://github.com/CMSgov/bluebutton-web-server/sessions/ec2649c7-d427-4f59-ad18-098613b75e4e Co-authored-by: bwang-icf <178809349+bwang-icf@users.noreply.github.com>
jimmyfagan
left a comment
jimmyfagan
left a comment
There was a problem hiding this comment.
Looks good, and seems to work as expected. Left a few more comments, should be good for a final approval after those last couple notes.
| code_challenge_method = request.POST.get('code_challenge_method') | ||
| validation_error = self._validate_code_challenge_method(code_challenge_method) | ||
| if validation_error: | ||
| return validation_error |
There was a problem hiding this comment.
Could we replace _validate_code_challenge_method with _check_for_required_params here too? If so, then let's get _validate_code_challenge_method removed altogether.
| return validation_error | ||
| kwargs['code_challenge'] = request.POST.get('code_challenge') | ||
| kwargs['code_challenge_method'] = request.POST.get('code_challenge_method') | ||
| kwargs['code_challenge_method'] = code_challenge_method |
There was a problem hiding this comment.
Looks like these indeed are not needed anymore, so we're good removing this. Some chance for related refactors in form_valid to uncomment the lines 398-399, and then removing the ifs for those values beneath that. Seems like these are all kind of remnants of when PKCE was first added, so some cleanup in this area would be nice while we're at it.
JIRA Ticket:
BB2-4805
What Does This PR Do?
Rejects any non S256 code challenge method calls
What Should Reviewers Watch For?
We already have some validation in the form of check_required_params and such within this file. Would it make sense to condense some of these?
Validation
Spin up a local instance and attempt an auth flow. Copy the authorize url and change the S256 value to something else. Observe a failed request and then attempt a regular workflow and see that it passes.
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval.
Any Migrations?
etc)